Skip to content

AddParamTypeBasedOnPHPUnitDataProviderRector: Enhance existing rule to handle PHPUnit 10+ DataProvider Attribute#4925

Merged
TomasVotruba merged 6 commits intorectorphp:mainfrom
mcampbell508:8179-AddParamTypeBasedOnPHPUnitDataProviderRector-attributes
Sep 30, 2023
Merged

AddParamTypeBasedOnPHPUnitDataProviderRector: Enhance existing rule to handle PHPUnit 10+ DataProvider Attribute#4925
TomasVotruba merged 6 commits intorectorphp:mainfrom
mcampbell508:8179-AddParamTypeBasedOnPHPUnitDataProviderRector-attributes

Conversation

@mcampbell508
Copy link
Copy Markdown
Contributor

The purpose of this PR is to hopefully address rectorphp/rector#8179.

I have tried to add atomic commits which should hopefully explain the changes made if you have time to review them commit by commit. I first started by adding failing tests as part of the first commit, which when running when checked out on that commit, fail.

I then refactored the existing code slightly to reuse the existing logic but with a renamed variable name, and then made the changes which I hope are along the right lines and within the expectations of the contributing guide.

The new method getPhpDataProviderAttribute, I added, is very similar to the existing code of

public function hasPhpAttribute(Property | ClassLike | ClassMethod | Param $node, string $attributeClass): bool
{
foreach ($node->attrGroups as $attrGroup) {
foreach ($attrGroup->attrs as $attribute) {
if (! $this->nodeNameResolver->isName($attribute->name, $attributeClass)) {
continue;
}
return true;
}
}
return false;
}
but I thought instead of doing a hasAttribute check, I would rather just loop through the attributes and collect what was needed (If the Attribute is the one we want here).

I am also not sure if we have to add a check that the version of PHP should be at least 8.0 for this feature, I tried to do so but the existing configured rule logic caused my tests to fail when having the following check in the rule:

    private function getPhpDataProviderAttribute($node): ?Attribute
    {
+        if (!$this->phpVersionProvider->isAtLeastPhpVersion(PhpVersionFeature::ATTRIBUTES)) {
+            return null;
+        }

because of the existing code:

I have left out the above isAtLeastPhpVersion check for now.

Other notes

  • Please see the commit and test I added for allowing only single data providers per test method. We may want to add this for the PHPDoc style of declaring data providers too, but I thought it may be out of scope for this PR, to do so. I hope you can see why this may be undesirable, because the types across different providers may be different - unless we did some sort of merging of all providers to determine the type.
  • I also added a rather minor housekeeping update for the docs to do with a missing ;, I can remove this if you would like it to be separate from this PR / want it gone.
  • I am not sure if I was supposed to include the built docs as part of the PR, which I have done.

Technical notes

PHPUnit 10 will first look for metadata in attributes before it looks at comments

which we probably want to do here.

  • An edge case that I may not have covered is for test methods that do the following (although it would be very unlikely but I guess could happen like anything 😄 ):
    /** @dataProvider provideData */
    #[\PHPUnit\Framework\Attributes\DataProvider('provideMoreData')]
    public function testAttributes($one, $two): void
    {
    }

    public static function provideData(): Iterator
    {
        yield ['string', 456];
    }

    public static function provideMoreData(): Iterator
    {
        yield [456, 'string'];
    }

Should we account for this?

Please let me know if you would like to make any changes, or please feel free to make any modifications to suit your needs/code style, if preferred.

I am fairly new to writing code in Rector but it is a rather enjoyable thing to work with.

#[\PHPUnit\Framework\Attributes\DataProvider('provideData')]
public function test_with_attribute(string $name)
{
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you separate these changes to a new fixture file? It will be easier to connect together in years ahead blame

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just friendly ping :)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi - apologies, have just pushed up some changes.

Separated all tests into new files in the same directory as the original tests.

@mcampbell508 mcampbell508 force-pushed the 8179-AddParamTypeBasedOnPHPUnitDataProviderRector-attributes branch 2 times, most recently from 856cd55 to c01370e Compare September 23, 2023 09:35
@mcampbell508
Copy link
Copy Markdown
Contributor Author

@TomasVotruba please be aware one test started failing after the push before last:

https://github.com/rectorphp/rector-src/actions/runs/6282888032/job/17062803915#step:5:97

I changed the test rules-tests/TypeDeclaration/Rector/ClassMethod/AddParamTypeBasedOnPHPUnitDataProviderRector/Fixture/skip_no_type_using_attribute.php.inc to instead do something else to cause the unknown type. Please let me know if there is any issue with this or you would just like me to remove the test.

Change:

     public static function provideData(): array
     {
         return [
-            [ new class {} ],
+            [ self::SOMETHING ],
         ];
     }
 }

We cannot do similar as the existing test not using the attribute because public static providers in PHPUnit 10+ do not allow arguments passed, I believe.

@mcampbell508 mcampbell508 force-pushed the 8179-AddParamTypeBasedOnPHPUnitDataProviderRector-attributes branch from c01370e to 4f67ab1 Compare September 29, 2023 14:34
Matt Campbell added 2 commits September 29, 2023 16:07
@mcampbell508 mcampbell508 force-pushed the 8179-AddParamTypeBasedOnPHPUnitDataProviderRector-attributes branch from 4f67ab1 to c813469 Compare September 29, 2023 15:09
Matt Campbell added 3 commits September 29, 2023 16:15
This should accomodate for the
`#[\PHPUnit\Framework\Attributes\DataProvider]` way of declaring data
providers now, from PHPUnit 10:

https://docs.phpunit.de/en/10.0/writing-tests-for-phpunit.html#data-providers
This test code was invalid when I pasted it into the online rector demo
tool originally, due to a missing semi colon.
After running: `composer run docs`
@mcampbell508 mcampbell508 force-pushed the 8179-AddParamTypeBasedOnPHPUnitDataProviderRector-attributes branch from c813469 to 3512af5 Compare September 29, 2023 15:15
@mcampbell508
Copy link
Copy Markdown
Contributor Author

mcampbell508 commented Sep 29, 2023

Hi @TomasVotruba - I have made some changes to accommodate for changes around this rule merged earlier today in #5068.

I had to refactor my original code slightly, but I think I have now accommodated for the changes @jlherren made. This includes adding 2 more tests around use of the attribute instead and also mixing between php doc and attribute.

The changes in #5068 also address some concerns I added in the description of this merge request involving multiple providers, which is nice!

I believe I also made all other changes around your comments.

Have requested a re-review, thanks.

I am not sure on how you prefer to review in terms of marking comments as resolved, for now I have left that up to you.

@TomasVotruba
Copy link
Copy Markdown
Member

Looks good, thank you 👍

@TomasVotruba TomasVotruba merged commit 0c0e5ee into rectorphp:main Sep 30, 2023
@mcampbell508 mcampbell508 deleted the 8179-AddParamTypeBasedOnPHPUnitDataProviderRector-attributes branch October 2, 2023 09:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants